-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue:3933 -Multi-node setup for local gradle run #3958
Issue:3933 -Multi-node setup for local gradle run #3958
Conversation
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
gradle/run.gradle
Outdated
if (project.hasProperty('nodeCount')) { | ||
useCluster testClusters.runMultiNodeTask; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can disrupt other plugins which use the same property. Should we use zoneCount
here and have runMultiZoneTask
instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 This sounds like a zone aware task. I would rather have it as runMultiZoneTask
Also curious, you've defined both tasks as runMultiNodeTask
. Isn't gradle yelling?
Thinking about probably these tasks are unnecessary and we can fold them to one task. Dropped another comment.
@@ -1239,6 +1243,7 @@ private void createConfiguration() { | |||
baseConfig.put("path.logs", confPathLogs.toAbsolutePath().toString()); | |||
baseConfig.put("path.shared_data", workingDir.resolve("sharedData").toString()); | |||
baseConfig.put("node.attr.testattr", "test"); | |||
baseConfig.put("node.attr.availabilityzone", zone); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should put config only for multi zone clusters .
Instead of passing this , can we use project.findProperty
instead here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure .. Let me try that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets fold them to one task and make it configurable. This will not conflict existing tasks defined by plugins.
Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
37b1f20
to
2db4aa4
Compare
Signed-off-by: Pranit Kumar <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Pranit Kumar <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are nice additions, is there a way we could test them?
Signed-off-by: Pranit Kumar <[email protected]>
At present couldn't think of an approach to test it. Also don't see ant test around single node run command. |
Gradle Check (Jenkins) Run Completed with:
|
buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchCluster.java
Show resolved
Hide resolved
@@ -1239,6 +1243,9 @@ private void createConfiguration() { | |||
baseConfig.put("path.logs", confPathLogs.toAbsolutePath().toString()); | |||
baseConfig.put("path.shared_data", workingDir.resolve("sharedData").toString()); | |||
baseConfig.put("node.attr.testattr", "test"); | |||
if (this.project.findProperty("numZones") != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need to check numZones
? if (zone != null && !zone.isBlank()) {
should be sufficient, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are passing zone value for the first node as well. Which will be true for the scenario where -PnumZones will not be set. Hence wanted to check the existence of the property. instead of zone variable.
Don't want to add zone attribute for the single node cluster. i.e
./gradlew run
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The decision should be made in OpenSearchCluster
: this class knows zones configuration (and should either associated node with zone or not).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the decision making to OpensearchCluster class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TY
…n OpensearchNode Signed-off-by: Pranit Kumar <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchCluster.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why call it numZones. All through the code base we use a generic term awareness attribute which could mean a rack/data centre/zone/switch
Signed-off-by: Pranit Kumar <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
I checked the code again . I see we are using term zone for awareness attribute. Couldn't think of any better way of classification. |
@pranikum looks like precommit is failing.
You can run './gradlew :build-tools:spotlessApply' to fix these violations. |
Signed-off-by: Pranit Kumar <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ Coverage Diff @@
## main #3958 +/- ##
============================================
- Coverage 70.55% 70.49% -0.06%
+ Complexity 57013 56974 -39
============================================
Files 4603 4603
Lines 274478 274496 +18
Branches 40201 40206 +5
============================================
- Hits 193658 193515 -143
- Misses 64511 64679 +168
+ Partials 16309 16302 -7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Done. |
@Bukhtawar , Can we please merge this PR. |
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
…#3958) * Issue:3933 - Multinode setup for local gradle run Signed-off-by: Pranit Kumar <[email protected]>
The PR contain changes for enhancing
gradle run
command to support multinode cluster on local. This change also take in zone count as project parameter for the gradle command. The nodes will be automatically distributed in zones. This will help us in testing multi node setup in local with debugging facilities.Issues Resolved
#3933
Execute ./gradlew run -PnumNodes=3 -PnumZones=3 to run 3 node cluster with 3 zones.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.